Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simple implementation and tests for #2191 #2220

Closed
wants to merge 2 commits into from
Closed

Conversation

Igmat
Copy link

@Igmat Igmat commented Dec 5, 2016

Summary

This PR implements #2191 issue from Jest repo itself and this one also kulshekhar/ts-jest#13.
In simple terms - it allows using Jest with TypeScript project just by adding "ts-jest": "latest" to devDependencies.

Test plan

All below configs should allow normal usage of Jest in TypeScript project.
Something like this one was used previously:

{
  "devDependencies": {
    "typescript": "latest",
    "jest": "latest",
    "ts-jest": "latest",
  },
  "jest": {
    "transform": {
      ".(ts|tsx)": "<rootDir>/node_modules/ts-jest/preprocessor.js"
    },
    "testResultsProcessor": "<rootDir>/node_modules/ts-jest/coverageprocessor.js",
    "testRegex": "(/__tests__/.*|\\.(test|spec))\\.(ts|tsx|js)$",
    "moduleFileExtensions": [
      "ts",
      "tsx",
      "js"
    ]
  }
}

With this PR config should look like this:

{
  "devDependencies": {
    "typescript": "latest",
    "jest": "latest",
    "ts-jest": "latest",
  }
}

Same behavior seems to work for babel-jest.

P.S.

It probably could also contain implementation for #2203 because it seems to be related, but I'm not sure. Could you please tell, should I implement it in this PR or make another one?

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@codecov-io
Copy link

codecov-io commented Dec 5, 2016

Current coverage is 89.36% (diff: 100%)

Merging #2220 into master will not change coverage

@@             master      #2220   diff @@
==========================================
  Files            39         39          
  Lines          1467       1467          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           1311       1311          
  Misses          156        156          
  Partials          0          0          

Powered by Codecov. Last update c3324a8...97bdaed

@Igmat
Copy link
Author

Igmat commented Dec 5, 2016

I probably have to create integration tests or you may want me to make some changes in ts-jest. If so, please, feel free to tell it here or create issue in ts-jest repo if it's more related to it than to this integration PR.

@Igmat
Copy link
Author

Igmat commented Dec 7, 2016

@cpojer could you, please, provide some feedback on this?

@cpojer
Copy link
Member

cpojer commented Dec 7, 2016

Hello! I will take a look when I have time. I work on Jest in open source mostly on my own time and I haven't had much time recently.

@Igmat
Copy link
Author

Igmat commented Dec 7, 2016

@cpojer, excuse me for hurrying you, I don't want to do that.

@Igmat
Copy link
Author

Igmat commented Dec 12, 2016

Should I resolve those new conflicts?

@cpojer
Copy link
Member

cpojer commented Dec 12, 2016

Hey, I was just finally able to take a look at this.

I'm not sure if we want to make ts-jest a default in Jest like this. First, I just refactored this test which was super annoying to work with, so any simplification you can make there is appreciated. Second, ts-jest is not really official but I'd like to figure out a way to make it work well with Jest without hardcoding so much. Do you have any suggestions to implement this better?

@Igmat
Copy link
Author

Igmat commented Dec 12, 2016

Ok, I have several ideas:

  1. I could try to move logic needed for ts-jest into separate file and call it from normalize.js, so code will be less noisy.
  2. I could move such logic into ts-jest itself, and leave only call for it from normalize.js, so this config always will be up to date for TypeScript projects.
  3. We could add something like externalConfig option to Jest config which will use provided path as default values, so anybody who wants to use ts-jest plugin will have to include only one option instead of 4-5 as it is now.

@cpojer
Copy link
Member

cpojer commented Dec 12, 2016

We do have a "preset" option – it only sucks if you want to use more than one, like TypeScript together with react-native. How about in addition we add a "presets" option that can take N number of presets? They may conflict, so it may not be a good idea.

@Igmat
Copy link
Author

Igmat commented Dec 12, 2016

Thanks for pointing to preset option, I somehow missed it.
It could be a good idea, but it seems to me that there would be a lot of issues. Mostly with "setupFiles".
Currently ts-jest doesn't provide any, but it could (e.g. to resolve this kulshekhar/ts-jest#79), also their order becomes very important.
After all current implementation will override important part of preset from ts-jest if it finds babel-jest due to this.

@cpojer
Copy link
Member

cpojer commented Jan 7, 2017

setupFiles from presets get merged with the local config, so that should work just fine.

@cpojer
Copy link
Member

cpojer commented Jan 8, 2017

@Igmat thank you for the PR and for the discussion here. Let's go back to the issue and take what we learned from this to make this a bit better. I do definitely want better ts-jest integration but I don't think this is the right way. I think we should try the preset way: let's expand the feature (see normalize.js) to support ts-jest well so we can make it easier to use. I think it is ok if it isn't automatic as long as it is simple to setup.

@cpojer cpojer closed this Jan 8, 2017
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants